Allow ll-specific multihash #1
Conversation
liamsi
force-pushed
the
liamsi/go-verifcid_allow_custom_mh
branch
from
9421478 to
c3e21cf
Compare
- and hack in that it accepts Sha256Namespace8Flagged = 0x7701
validate.go
Outdated
| // XXX: This has to be on par with what the LazyLedger IPLD plugin registers as a multihash | ||
| // namely, Sha256Namespace8Flagged. We simply repeat the constant here instead of | ||
| // importing the corresponding package from lazyledger-core as this would be overkill. | ||
| if code == 0x7701 { | ||
| return true | ||
| } |
There was a problem hiding this comment.
An alternative would be to import the constant from where it is defined, currently in lazyledger-core:
https://github.com/lazyledger/lazyledger-core/blob/755a7175168e57e9f1996341e533683eace5c5d3/p2p/ipld/plugin/nodes/nodes.go#L27-L29
For now it seems more reasonable to repeat this here instead of introducing a massive dependency and assert that this checks out by the consumer of verifcid:
https://github.com/lazyledger/lazyledger-core/blob/7b9550659e2bd957b4042880b8339d9e1d84c9c0/p2p/ipld/plugin/nodes/nodes_test.go#L26-L30
There was a problem hiding this comment.
Another option is to extract "common" dependency. But creating a module just for single const seems like overengineering.
There was a problem hiding this comment.
Thought about this too. But I agree, it would be overkill for now.
There was a problem hiding this comment.
Is it possible to make this use of a magic number a bit more...documented (can't think of a better word)? I.e. this file documents what's happening, but if you go modify the magic number the other file you have no idea this one exists. Even putting it in the PR's description instead of a comment would go a long way.
There was a problem hiding this comment.
OK, I'll put it in the PR description.
BTW, there is a test in the consuming code that would fail if the constant was modified without updating it here accordingly: https://github.com/lazyledger/lazyledger-core/blob/3308e7636503ded830e476cd79ec4f9a5cf16de0/p2p/ipld/plugin/nodes/nodes_test.go#L26-L30
liamsi
changed the title
liamsi
requested review from
adlerjohn,
evan-forbes,
tac0turtle,
musalbas and
tzdybal
validate.go
Outdated
| // XXX: This has to be on par with what the LazyLedger IPLD plugin registers as a multihash | ||
| // namely, Sha256Namespace8Flagged. We simply repeat the constant here instead of | ||
| // importing the corresponding package from lazyledger-core as this would be overkill. | ||
| if code == 0x7701 { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Another option is to extract "common" dependency. But creating a module just for single const seems like overengineering.
This change is necessary if we want IPFS to allow our custom hasher that is introduced and used in celestiaorg/celestia-core#155 and celestiaorg/go-ipfs#1
Note: this PR simply duplicates a constant introduced in another repo. namely,
0x7701. It corresponds to the custom hasher code defined and registered in the LazyLedger ipld plugin. The calling code makes sure via a test, that a changing the constant only there wouldn't go unnoticed.